Performance improvements #445
Conversation
|
Even with these changes, it's still a lot of calls to helm. There must be a way of doing this that minimises calls to helm further by design, but I'm quite busy so I can't think of it yet |
|
<removed comment when i discovered |
| versionsC := make(chan [4]string, len(s.Apps)) | ||
|
|
||
| // We store the results of the helm commands | ||
| extractedChartNames := make(map[string]string) |
There was a problem hiding this comment.
probably using a struct for the extracted chart names and versions would be cleaner but I guess we can do that in another iteation
There was a problem hiding this comment.
I went through your code with debugger to make sure I correctly get your whole idea.
I really like what you did with Dockerfile. This should be even separate PR so it goes quicker.
There are really as you said in the code's comments places that violates DRY (not that we were not doing this before), but let's push it and I hope at some point this is getting fixed too. We just don't want anyone to lose motivation. :-)
Looks good overall, but due to code's repeat it complicated the reading much and at some point I was concerned. Will you please take a look at comments I left before we can go with it any further?
internal/app/helm_helpers.go
Outdated
| @@ -53,6 +53,8 @@ func getHelmVersion() string { | |||
| if result.code != 0 { | |||
| log.Fatal("While checking helm version: " + result.errors) | |||
| } | |||
|
|
|||
| log.Verbose("Helm version " + result.output) | |||
There was a problem hiding this comment.
there's no need to log that since the helm version is printed in cli.go:152
|
@mkubaczyk i've seen your comments, i'll address them and clean this up in a day or so |
|
Please do. We are waiting with the release for your changes to join it as well. :-) |
|
@mkubaczyk fixed your comments. I want to cache the downloaded tars that Docker |
| @@ -25,8 +25,8 @@ func helmCmd(args []string, desc string) command { | |||
| } | |||
|
|
|||
| // extractChartName extracts the Helm chart name from full chart name in the desired state. | |||
| func extractChartName(releaseChart string, version string) string { | |||
| cmd := helmCmd([]string{"show", "chart", releaseChart, "--version", version}, "Show chart information for version "+version) | |||
| func extractChartName(releaseChart string) string { | |||
There was a problem hiding this comment.
I reverted this change -- we no longer use this when we already have a version
|
@mkubaczyk i've addressed your feedback and it looks like it passes the build again |
|
good, thanks! I'm merging it :-) |
|
@mkubaczyk a point of note: we should (I can, in a new PR) update the Dockerfile again and cache the initial container somehow, but only when we update the versions -- it would vastly improve the Docker layering to do so |
|
@fmd whatever you feel is right for the codebase, please propose it with MR. We love to see new things coming into the repo :-) |
In short, this PR makes
helmsmancallhelmless.If we have 100 apps, and they are all on the same version of the same chart, helmsman would validate that chart 100 times in a row with identical calls to
helm. This doesn't need to happen. I kept the idea of running helm commands concurrently aresourcePoolnumber of times (more onresourcePoolvs the-pflag later in this comment), but I have expanded it -- we concurrently ask helm about the charts (like we used to), but we only do so once per chart version pair (at least inmakePlan).Compared to my previous PR, I am no longer trying to cache chart names and versions in an atomic
sync.Cache. Since we went to the OS and called helm a bunch of times concurrently, they'd all check the cache and see the result wasn't there, then all the resources in the pool would attempt to fill the cache by calling the expensivehelmcommand at the same time. It was still faster (because I decoupledvalidateChartfrom the release), but this is faster still.I've updated the Dockerfile quite a bit too, I don't know if it'll pass CircleCI.
For our use case these changes seem to roughly double the speed of our deploys on helmsman. However, I haven't done any profiling (just a few very quick eyeball tests) and I haven't written any tests that test when there's lots of releases on a mixture of charts.
resourcePoolnow applies slightly differently to makePlan. I suggest we combineresourcePoolwith the-pflag so we can set the resourcePool number ourselves and have it apply consistently, but another time I want to look at how the concurrency on helm commands is performing across the board. Some helm commands still aren't run concurrently (liker.checkChartDepUpdate()which I haven't looked at yet), and i have no data on what kind ofresourcePoolnumber might be optimal, but this is a start.This could definitely be DRYed up and refactored but I think these changes should provide some inroads into making helmsman more performant, especially when someone is deploying a lot of apps that share charts (as we are).